Add cronjob for syncing heartbeats with heartbeat-missing incidents#1954
Open
hmpf wants to merge 6 commits into
Open
Add cronjob for syncing heartbeats with heartbeat-missing incidents#1954hmpf wants to merge 6 commits into
hmpf wants to merge 6 commits into
Conversation
Contributor
Author
Oopsies discovered
|
Test results 8 files 1 848 suites 3m 48s ⏱️ Results for commit ae446d0. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1954 +/- ##
==========================================
+ Coverage 88.79% 89.04% +0.24%
==========================================
Files 152 154 +2
Lines 7276 7466 +190
==========================================
+ Hits 6461 6648 +187
- Misses 815 818 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8 tasks
hmpf
changed the title
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
3 times, most recently
from
9edb660 to
2716a93
Compare
hmpf
force-pushed
the
duration-field-for-heartbeats
branch
from
9819f90 to
2526843
Compare
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
from
2716a93 to
210f86e
Compare
hmpf
force-pushed
the
create_fake_incident-refactor
branch
2 times, most recently
from
8ebdaa5 to
39de931
Compare
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
from
210f86e to
4fee3d0
Compare
|
Warning 2 of 38 new test names are missing convention keywords (
Why am I seeing this?Test names should follow a loose given/when/then pattern with keywords like Examples:
|
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
from
4fee3d0 to
f708f73
Compare
hmpf
force-pushed
the
create_fake_incident-refactor
branch
from
9b35864 to
fbe1b88
Compare
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
2 times, most recently
from
a220d49 to
8e9fc88
Compare
hmpf
force-pushed
the
create_fake_incident-refactor
branch
from
fbe1b88 to
0e6c980
Compare
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
from
8e9fc88 to
8c75ae1
Compare
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
2 times, most recently
from
0880eb7 to
d342583
Compare
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
from
d342583 to
27163f8
Compare
hmpf
force-pushed
the
heartbeat-checker-cronjob
branch
from
27163f8 to
e99c801
Compare
stveit
reviewed
Jul 1, 2026
stveit
left a comment
stveit
left a comment
Contributor
There was a problem hiding this comment.
Posting this as "comment" since one comment is a nitpick and the other might be a "request change", but im not entirely sure about the intentions of the code
hmpf
commented
Jul 2, 2026
hmpf
commented
Jul 2, 2026
hmpf
commented
Jul 2, 2026
hmpf
commented
Jul 2, 2026
hmpf
commented
Jul 2, 2026
hmpf
commented
Jul 2, 2026
hmpf
commented
Jul 2, 2026
lunkwill42
reviewed
Jul 2, 2026
Comment on lines
108
to
+109
| def TestCreateStatelessIncident(TestCase): | ||
| def test_create_stateless_incident(self): | ||
| def test_it_should_always_return_a_stateless_incident(self): |
Member
There was a problem hiding this comment.
This test class was b0rked before this PR ....
Member
There was a problem hiding this comment.
A.K.A. Always start with empty tests, run them and observe they are failing (so you know they have been discovered by the test runner) - then implement them.
johannaengland
requested changes
Jul 2, 2026
| def _close_incidents_whose_sources_are_alive_again(timestamp): | ||
| argus = SourceSystem.objects.get(name="argus") | ||
| still_dead_sources = SourceSystem.objects.dead(timestamp - _FUDGE) | ||
| outdated_incidents = Incident.objects.heartbeat_incidents().open() |
Contributor
There was a problem hiding this comment.
I would call this rather open_heartbeat_incidents - because we don't know if they are outdated yet
Contributor
Author
There was a problem hiding this comment.
Call what? Which of the four lines?
Contributor
There was a problem hiding this comment.
outdated_incidents -> open_heartbeat_incidents
|
|
||
|
|
||
| def _create_incidents_for_dead_sources(timestamp: Optional[datetime] = None): | ||
| # Create existing incidents whose sources have become dead |
Contributor
There was a problem hiding this comment.
This comment confuses me - are you creating them or are they existing? And this should be a docstring
| timestamp: Optional[datetime] = None, | ||
| level: int = DEFAULT_LEVEL, | ||
| ): | ||
| assert isinstance(source, SourceSystem), "source is not a SourceSystem" |
Contributor
There was a problem hiding this comment.
Suggested change
| assert isinstance(source, SourceSystem), "source is not a SourceSystem" | |
| assert isinstance(source, SourceSystem), f"source {source} is not a SourceSystem" |
| return qs | ||
|
|
||
| def dead(self, timestamp: Optional[datetime] = None): | ||
| """Find sources that have missed heartbeats |
Contributor
There was a problem hiding this comment.
Might be helpful to mention in the docstring what the timestamp means in this context
| self.assertFalse(result) | ||
|
|
||
| def test_when_incident_has_both_magical_heartbeat_tags_return_True(self): | ||
| source_name = "hearteat-source" |
Contributor
There was a problem hiding this comment.
Suggested change
| source_name = "hearteat-source" | |
| source_name = "heartbeat-source" |
| self.assertNotIn(tag3, result) | ||
| self.assertEqual(set((tag1, tag2)), result) | ||
|
|
||
| def test_given_existing_tags_from_tag_keys_returns_a_queryset_of_tags_the_given_tag_key(self): |
Contributor
There was a problem hiding this comment.
Suggested change
| def test_given_existing_tags_from_tag_keys_returns_a_queryset_of_tags_the_given_tag_key(self): | |
| def test_given_existing_tags_from_tag_keys_returns_a_queryset_of_tags_with_the_given_tag_key(self): |
|
|
||
| @tag("db") | ||
| class TestIncidentQuerySetTagMethods(TestCase): | ||
| def test_when_no_incident_has_source_with_key_return_empty_queryset(self): |
| @@ -0,0 +1,4 @@ | |||
| Added management command suitable to run as a cronjob that looks for dead | |||
| incidents (as per heartbeat freqeuncy and last seen heartbeat). Dead sources | |||
Contributor
There was a problem hiding this comment.
Suggested change
| incidents (as per heartbeat freqeuncy and last seen heartbeat). Dead sources | |
| incidents (as per heartbeat frequency and last seen heartbeat). Dead sources |
github-project-automation
Bot
moved this from ❓ Ready for review
to ♻ Changes requested
in Argus development, public
|
hmpf
moved this from ♻ Changes requested
to ❓ Ready for review
in Argus development, public
| DEFAULT_LEVEL = 4 | ||
| _FUDGE = timedelta(seconds=DEFAULT_FUDGE_FACTOR) | ||
| HEARTBEAT_FUDGE = timedelta(seconds=DEFAULT_FUDGE_FACTOR) | ||
| INCIDENT_DESCRIPTION_TEMPLATE = "Missing heartbeat from source {source}, dead?" |
Contributor
There was a problem hiding this comment.
Suggested change
| INCIDENT_DESCRIPTION_TEMPLATE = "Missing heartbeat from source {source}, dead?" | |
| INCIDENT_DESCRIPTION_TEMPLATE = "Missing heartbeat from source {source} ({type}), dead?" |
| def _close_heartbeat_incidents(timestamp: datetime) -> Tuple[List[SourceSystem], List[Incident], List[Incident]]: | ||
| """Close all existing heartbeat incidents that can be closed | ||
|
|
||
| Returns a list of reanimated sources foll: owed by a list of freshly |
Contributor
There was a problem hiding this comment.
Suggested change
| Returns a list of reanimated sources foll: owed by a list of freshly | |
| Returns a list of reanimated sources followed by a list of freshly |
| """Close all existing heartbeat incidents that can be closed | ||
|
|
||
| Returns a list of reanimated sources foll: owed by a list of freshly | ||
| created incidents and ifinally a list of incidents that could not be closed. |
Contributor
There was a problem hiding this comment.
Suggested change
| created incidents and ifinally a list of incidents that could not be closed. | |
| created incidents and finally a list of incidents that could not be closed. |
| return sources, closed_incidents, remaining_incidents | ||
|
|
||
|
|
||
| def _attempt_closing_heartbeat_incident( |
Contributor
There was a problem hiding this comment.
Great idea to factor this out!
| freshly created (True) or not (False). | ||
|
|
||
| Logs if there are multiple incidents for the source, and returns one of the | ||
| sources and that it was not feshly created (False). |
Contributor
There was a problem hiding this comment.
Suggested change
| sources and that it was not feshly created (False). | |
| incidents and that it was not freshly created (False). |
| # @transaction.atomic | ||
| def set_closed(self, actor: User, timestamp: datetime = None, description=""): | ||
| "Incident closed by user" | ||
| "Incident closed by human user" |
Contributor
There was a problem hiding this comment.
Suggested change
| "Incident closed by human user" | |
| "Closes incident as a human user" | |
| `` |
| # @transaction.atomic | ||
| def set_end(self, actor: User, timestamp: datetime = None, description: str = ""): | ||
| "Incident closed by source" | ||
| "Incident ended by machine source" |
Contributor
There was a problem hiding this comment.
Suggested change
| "Incident ended by machine source" | |
| "Ends incident as a machine source" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope and purpose
Fixes #1947
Depends on #1962
Revieweing tips
The first commit does the heavy lifting, the second adds the actual command.
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Argus can be found in the
Development docs.